Re: [Xen-devel] [PATCH v4 1/9] xen/vpci: introduce basic handlers to trap accesses to the PCI config space

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

On Fri, Jul 14, 2017 at 10:01:54AM -0600, Jan Beulich wrote:
> >>> On 14.07.17 at 17:33, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, Jul 13, 2017 at 08:36:18AM -0600, Jan Beulich wrote:
> >> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:01 PM >>>
> >> > +#define container_of(ptr, type, member) ({                      \
> >> > +        typeof(((type *)0)->member) *__mptr = (ptr);            \
> >> > +        (type *)((char *)__mptr - offsetof(type, member));      \
> >> 
> >> I don't know what tools maintainers think about such name space
> >> violations; in hypervisor code I'd ask you to avoid leading underscores
> >> in macro local variables (same in min()/max() and elsewhere then).
> > 
> > OK. container_of, max and min and verbatim copies of the macros in
> > xen/include/xen/kernel.h, with the style adjusted in the container_of
> > case IIRC (as requested in the previous review).
> 
> Well, that's one of the frequent problems we have: People copy and
> paste things without questioning them. We only make things worse if
> we clone code we wouldn't permit in anymore nowadays.

Sorry, that comment sounded like a justification, which is not
intended. I was just explaining how that ended up there the way it
is.

> >> > +    /*
> >> > +     * Test all possible read/write size combinations.
> >> > +     *
> >> > +     * Populate 128bits (16B) with 1B registers, 160bits (20B) with 2B
> >> > +     * registers, and finally 192bits (24B) with 4B registers.
> >> 
> >> I can't see how the numbers here are in line with the code this is
> >> meant to describe. Perhaps this is a leftover from an earlier variant
> >> of the code?
> > 
> > I'm not sure I understand this, the registers (or layout) described in
> > this comment are just added below the comment. Would you like me to
> > first add the registers and place the comment afterwards?
> 
> No, my point is that code that follows this doesn't populate as
> many bits as the comment says. From what I understand, you
> use 4 byte registers, 2 word ones, and one dword one.

OK, I think I see what you mean. The comment makes it looks I'm
populating 128bits, which what I indented to say is:

[...]
 * Place 4 1B registers at 128bits (16B), 2 2B registers at 160bits (20B)
 * and finally 1 4B register at 192bits (24B).

> >> > --- a/xen/arch/arm/xen.lds.S
> >> > +++ b/xen/arch/arm/xen.lds.S
> >> > @@ -41,6 +41,9 @@ SECTIONS
> >> >  
> >> >    . = ALIGN(PAGE_SIZE);
> >> >    .rodata : {
> >> > +       __start_vpci_array = .;
> >> > +       *(.rodata.vpci)
> >> > +       __end_vpci_array = .;
> >> 
> >> Do you really need this (unconditionally)?
> > 
> > Right, this should have a ifdef CONFIG_PCI.
> 
> CONFIG_HAS_PCI for one, and then ARM doesn't select this at
> all. Hence the question.

I think it would be better to just add it now? The code is not really
x86 specific (although it's only used by x86 ATM). IMHO adding a
CONFIG_HAS_PCI to both linker scripts is the best solution.

> >> > +static int vpci_access_check(unsigned int reg, unsigned int len)
> >> 
> >> The way you use it, this function want to return bool.
> >> 
> >> > +void hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
> >> > +                         unsigned int *bus, unsigned int *slot,
> >> > +                         unsigned int *func, unsigned int *reg)
> >> 
> >> Since you return nothing right now, how about avoid one of the
> >> indirections? Best candidate would probably be the register value.
> > 
> > I don't really like functions that return some data in the return
> > value (if it's not an error code) and some other data in parameters.
> 
> Well, okay, I view it the other way around - return by indirection
> is to be used if return by value is not reasonable (too much data).
> Hence it's kind of an overflow to me, not a replacement.
> 
> >> > +int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> >> 
> >> As pointed out in reply to an earlier version, this lacks a prereq
> >> change: setup_one_hwdom_device() needs to be marked __hwdom_init. And
> >> then, now that you have the annotation here, the placement of the
> >> array in the linker script should depend on whether __hwdom_init is an
> >> alias of __init.
> > 
> > The __hwdom_init prefix is dropped shortly from this function (patch
> > #3), but I agree on sending a pre-patch to address
> > setup_one_hwdom_device.
> 
> I have one ready, btw.
> 
> > The linker script I'm not sure it's worth modifying, by the end of the
> > series the list of handlers must reside in .rodata.
> 
> As per the reply to that later patch, I'm not yet convinced that
> these annotations will go away. Hence I'd prefer if things were
> handled fully correctly here.
> 
> >> > +static uint32_t vpci_read_hw(unsigned int seg, unsigned int bus,
> >> > +                             unsigned int slot, unsigned int func,
> >> > +                             unsigned int reg, uint32_t size)
> >> > +{
> >> > +    uint32_t data;
> >> > +
> >> > +    switch ( size )
> >> > +    {
> >> > +    case 4:
> >> > +        data = pci_conf_read32(seg, bus, slot, func, reg);
> >> > +        break;
> >> > +    case 2:
> >> > +        data = pci_conf_read16(seg, bus, slot, func, reg);
> >> > +        break;
> >> > +    case 1:
> >> > +        data = pci_conf_read8(seg, bus, slot, func, reg);
> >> > +        break;
> >> > +    default:
> >> > +        BUG();
> >> 
> >> As long as this is Dom0-only, BUG()s like this are probably fine, but
> >> if this ever gets extended to DomU-s, will we really remember to
> >> convert them?
> > 
> > ASSERT_UNREACHABLE() and set data to ~0 to be safe?
> 
> Yes please.
> 
> >> > +     */
> >> > +    data = vpci_read_hw(seg, bus, slot, func, reg, size);
> >> 
> >> I continue to be worried of reads that have side effects here. Granted
> >> we currently don't emulate any, but it would feel better if we didn't
> >> do the read for no reason. I.e. do hw reads only to fill gaps between
> >> emulated fields.
> > 
> > Heh, right. I got this "idea" from pciback, but I will change it so
> > the logic is similar to the write one (which obviously doesn't write
> > everything and then checks for emulated registers).
> > 
> > As a side question, which kind of registers have read side effects on
> > PCI? Reading the spec (PCIe 3.1A) there's no type of register listed
> > in section 7.4 (ro, rw, rw1c and the sticky versions) that mentions
> > read side effects. Is that described somewhere for specific
> > registers?
> 
> I don't think there are any specified, but iirc a well known side effect
> of VPD reads from some cards is that it'll hang the box for certain
> (normally invalid) indexes. As said, we don't emulate anything like
> that, but let's be defensive wrt hardware quirks.

Thanks for the clarification.

Roger.

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