Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 04, 2017 at 08:30:38AM +0000, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, <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;
> > +    unsigned int reg;
> > +    pci_sbdf_t sbdf;
> > +    uint32_t cf8;
> > +
> > +    *data = ~(uint64_t)0;
> > +
> > +    if ( addr == 0xcf8 )
> > +    {
> > +        ASSERT(size == 4);
> > +        *data = d->arch.hvm_domain.pci_cf8;
> > +        return X86EMUL_OKAY;
> > +    }
> > +
> > +    cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
> > +    if ( !CF8_ENABLED(cf8) )
> > +        return X86EMUL_OKAY;
> 
> Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
> forwarded to qemu if it's not a config space one. Same in the write path
> then.

No, I don't think this should be forwarded to QEMU. It is a config
space access (because vpci_portio_accept returned true). But the value
in CF8 doesn't have the enabled bit set, hence the access is
discarded.

> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -124,6 +124,12 @@ SECTIONS
> >         __param_start = .;
> >         *(.data.param)
> >         __param_end = .;
> > +
> > +#if defined(CONFIG_HAS_PCI) && defined(CONFIG_LATE_HWDOM)
> > +       __start_vpci_array = .;
> > +       *(.data.vpci)
> > +       __end_vpci_array = .;
> > +#endif
> >    } :text
> >  
> >  #if defined(BUILD_ID)
> > @@ -213,6 +219,12 @@ SECTIONS
> >         *(.init_array)
> >         *(SORT(.init_array.*))
> >         __ctors_end = .;
> > +
> > +#if defined(CONFIG_HAS_PCI) && !defined(CONFIG_LATE_HWDOM)
> > +       __start_vpci_array = .;
> > +       *(.data.vpci)
> > +       __end_vpci_array = .;
> > +#endif
> >    } :text
> 
> Suitable alignment needs to be enforced in both cases, or else we risk
> someone adding something immediately ahead of one of your insertions,
> making __start_vpci_array no longer point to the first entry.

OK, I've used . = ALIGN(POINTER_ALIGN); for both x86 and ARM.

Thanks, Roger.

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