Re: [Xen-devel] [PATCH v7 for-next 03/12] vpci: introduce basic handlers to trap accesses to the PCI config space

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.17 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> +static int vpci_portio_read(const struct hvm_io_handler *handler,
> +                            uint64_t addr, uint32_t size, uint64_t *data)
> +{
> +    struct domain *d = current->domain;

const?

> +    unsigned int reg;
> +    pci_sbdf_t sbdf;
> +    uint32_t cf8;
> +
> +    *data = ~(uint64_t)0;
> +
> +    if ( addr == 0xcf8 )
> +    {
> +        ASSERT(size == 4);

With this checking what vpci_portio_accept() did, ...

> +        *data = d->arch.hvm_domain.pci_cf8;
> +        return X86EMUL_OKAY;
> +    }
> +
> +    cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);

... how about also having

    ASSERT((addr & ~3) == 0xcfc);

ahead of this line? Same for vpci_portio_write() then.

> +static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg,
> +                             unsigned int size)
> +{
> +    uint32_t data;
> +
> +    switch ( size )
> +    {
> +    case 4:
> +        data = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, reg);
> +        break;
> +    case 3:
> +        /*
> +         * This is possible because a 4byte read can have 1byte trapped and
> +         * the rest passed-through.
> +         */
> +        if ( reg & 1 )
> +        {
> +            data = pci_conf_read8(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
> +                                  reg);
> +            data |= pci_conf_read16(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
> +                                    reg + 1) << 8;
> +        }
> +        else
> +        {
> +            data = pci_conf_read16(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
> +                                   reg);
> +            data |= pci_conf_read8(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
> +                                   reg + 2) << 16;
> +        }
> +        break;
> +    case 2:
> +        data = pci_conf_read16(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, reg);
> +        break;
> +    case 1:
> +        data = pci_conf_read8(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, reg);
> +        break;
> +    default:
> +        ASSERT_UNREACHABLE();
> +        data = ~(uint32_t)0;
> +        break;
> +    }

Blank lines between non-fall-through case blocks please (also
elsewhere in the series if applicable).

With these cosmetic items taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel