Re: [Xen-devel] [PATCH v6 03/11] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
> +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
> +                           unsigned int len, unsigned long *data)
> +{
> +    struct domain *d = v->domain;
> +    const struct hvm_mmcfg *mmcfg;
> +    unsigned int reg;
> +    pci_sbdf_t sbdf;
> +
> +    *data = ~0ul;
> +
> +    read_lock(&d->arch.hvm_domain.mmcfg_lock);
> +    mmcfg = vpci_mmcfg_find(d, addr);
> +    if ( !mmcfg )
> +    {
> +        read_unlock(&d->arch.hvm_domain.mmcfg_lock);
> +        return X86EMUL_OKAY;
> +    }

With the lock dropped between accept() and read() (or write() below),
is it really appropriate to return OKAY here? The access again should
be forwarded to qemu, I would think.

> +int __hwdom_init register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> +                                             unsigned int start_bus,
> +                                             unsigned int end_bus,
> +                                             unsigned int seg)
> +{
> +    struct hvm_mmcfg *mmcfg;
> +
> +    ASSERT(is_hardware_domain(d));
> +
> +    write_lock(&d->arch.hvm_domain.mmcfg_lock);
> +    if ( vpci_mmcfg_find(d, addr) )
> +    {
> +        write_unlock(&d->arch.hvm_domain.mmcfg_lock);
> +        return -EEXIST;
> +    }

You check here for an exact match in starting address. Is it really
to reject just this special case, rather than doing a proper overlap
check?

> +    mmcfg = xmalloc(struct hvm_mmcfg);

Whenever possible without too much trouble allocations should be done
with no lock held.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel