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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>> > +{
>> > +    unsigned int i;
>> > +
>> > +    /* Write using bytes. */
>> > +    for ( i = 0; i < 4; i++ )
>> > +        VPCI_WRITE_CHECK(reg + i, 1, (val >> (i * 8)) & UINT8_MAX);
>> > +    multiread4(reg, val);
>> > +
>> > +    /* Write using 2bytes. */
>> > +    for ( i = 0; i < 2; i++ )
>> > +        VPCI_WRITE_CHECK(reg + i * 2, 2, (val >> (i * 2 * 8)) & 
>> > UINT16_MAX);
>> > +    multiread4(reg, val);
>> > +
>> > +    VPCI_WRITE_CHECK(reg, 4, val);
>> > +    multiread4(reg, val);
>> > +}
>> 
>> Wouldn't it be better to vary the value written between the individual
>> sizes? Perhaps move the 32-bit write between the two loops, using ~val?
>> Otherwise you won't know whether what you read back is a result of the
>> writes you actually mean to test or earlier ones?
> 
> So storing a new value in val between each size test? I could even use
> something randomly generated.

Random data is bad for reproducibility (if e.g. you want to debug a
case where the test suddenly fails).

>> > +    /*
>> > +     * 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.

>> > --- 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.

>> > +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.

>> > +int __must_check vpci_add_register(const struct pci_dev *pdev,
>> > +                                   vpci_read_t read_handler,
>> > +                                   vpci_write_t write_handler,
>> 
>> I'm surprised this compiles without (at least) warnings - you appear to
>> be lacking *s here.
> 
> I think in the previous version the type itself had a pointer, and
> then I removed it and haven't updated it here. But yes, none of the
> compilers seems to complain:
> 
> https://travis-ci.org/royger/xen/builds/248811315 
> 
> Is it maybe implicit that function types are pointers?

Well, maybe I'm wrong with my assumption that this formally is
illegal.

Jan

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