|
From: | Pierre Morel |
Subject: | Re: [Qemu-devel] [PATCH v2 2/7] s390x/pci: rework PCI STORE |
Date: | Wed, 22 Nov 2017 22:12:08 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 22/11/2017 14:25, Cornelia Huck wrote:
On Tue, 21 Nov 2017 19:03:17 +0100 Pierre Morel <address@hidden> wrote:On 21/11/2017 15:25, Cornelia Huck wrote:On Tue, 21 Nov 2017 11:38:45 +0100 Cornelia Huck <address@hidden> wrote:On Thu, 16 Nov 2017 18:51:50 +0100 Pierre Morel <address@hidden> wrote:@@ -493,9 +494,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) break; }- data = env->regs[r1];- if (pcias < 6) { - if ((8 - (offset & 0x7)) < len) { + /* Test that the pcias is valid and do the appropriates operations */ + switch (pcias) { + case 0 ... 5:Make this case 0...5: ?...only that it confuses the compiler when using numbers. We can eitherI did not see this as I replied to the previous email, sorry.keep the slightly ugly version, or introduce #defines. ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?I agree something is missing. But I am not sure that a #define brings clarity. I would prefer to add a comment. /* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */ ?It's more a speaking value vs. magic numbers thing. A comment does not hurt either, though :) (And we get rid of the spacing as well.)
OK, thanks.
+ * A length of 0 is invalid and length should not cross a double word + */ + if (!len || (len > (8 - (offset & 0x7)))) { program_interrupt(env, PGM_OPERAND, 4); return 0; } @@ -513,21 +518,21 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) program_interrupt(env, PGM_OPERAND, 4); return 0; } - } else if (pcias == 15) { - if ((4 - (offset & 0x3)) < len) { - program_interrupt(env, PGM_OPERAND, 4); - return 0; - } - - if (zpci_endian_swap(&data, len)) { + break; + case 15: + /* ZPCI uses the pseudo BAR number 15 as configuration space */ + /* possible access lengths are 1,2,4 and must not cross a word */ + if (!len || (len > (4 - (offset & 0x3))) || len == 3) { program_interrupt(env, PGM_OPERAND, 4); return 0; } - + /* len = 1,2,4 so we do not need to test */ + zpci_endian_swap(&data, len); pci_host_config_write_common(pbdev->pdev, offset, pci_config_size(pbdev->pdev), data, len); - } else { + break; + default: DPRINTF("pcistg invalid space\n"); setcc(cpu, ZPCI_PCI_LS_ERR); s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);Other than the nits, looks good.
-- Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
[Prev in Thread] | Current Thread | [Next in Thread] |