[Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Jan Beulich]

Expecting the ROM BAR to be written with an all ones value when sizing
the region is wrong - the low bit has another meaning (enable/disable)
and bits 1..10 are reserved. The PCI spec also mandates writing all
ones to just the address portion of the register.

Use suitable constants also for initializing the ROM BAR register field
description.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
 
     /* check unused BAR register */
     index = xen_pt_bar_offset_to_index(addr);
-    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
+    if ((index >= 0) && (val != 0) &&
+        (((index != PCI_ROM_SLOT) ?
+          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) &&
         (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
         XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address "
                     "Register. (addr: 0x%02x, len: %d)\n", addr, len);
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade
         .offset     = PCI_ROM_ADDRESS,
         .size       = 4,
         .init_val   = 0x00000000,
-        .ro_mask    = 0x000007FE,
-        .emu_mask   = 0xFFFFF800,
+        .ro_mask    = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE,
+        .emu_mask   = (uint32_t)PCI_ROM_ADDRESS_MASK,
         .init       = xen_pt_bar_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
         .u.dw.write = xen_pt_exp_rom_bar_reg_write,

[Qemu-devel] Ping: [PATCH] xen/pass-through: ROM BAR handling adjustments

[Jan Beulich]

Ping?

>>> On 15.05.15 at 14:41, <JBeulich@suse.com> wrote:
> Expecting the ROM BAR to be written with an all ones value when sizing
> the region is wrong - the low bit has another meaning (enable/disable)
> and bits 1..10 are reserved. The PCI spec also mandates writing all
> ones to just the address portion of the register.
> 
> Use suitable constants also for initializing the ROM BAR register field
> description.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
>  
>      /* check unused BAR register */
>      index = xen_pt_bar_offset_to_index(addr);
> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
> +    if ((index >= 0) && (val != 0) &&
> +        (((index != PCI_ROM_SLOT) ?
> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != 
> XEN_PT_BAR_ALLF) &&
>          (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
>          XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address 
> "
>                      "Register. (addr: 0x%02x, len: %d)\n", addr, len);
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade
>          .offset     = PCI_ROM_ADDRESS,
>          .size       = 4,
>          .init_val   = 0x00000000,
> -        .ro_mask    = 0x000007FE,
> -        .emu_mask   = 0xFFFFF800,
> +        .ro_mask    = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE,
> +        .emu_mask   = (uint32_t)PCI_ROM_ADDRESS_MASK,
>          .init       = xen_pt_bar_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
>          .u.dw.write = xen_pt_exp_rom_bar_reg_write,
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Stefano Stabellini]

On Fri, 15 May 2015, Jan Beulich wrote:
> Expecting the ROM BAR to be written with an all ones value when sizing
> the region is wrong - the low bit has another meaning (enable/disable)
> and bits 1..10 are reserved. The PCI spec also mandates writing all
> ones to just the address portion of the register.
> 
> Use suitable constants also for initializing the ROM BAR register field
> description.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
>  
>      /* check unused BAR register */
>      index = xen_pt_bar_offset_to_index(addr);
> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
> +    if ((index >= 0) && (val != 0) &&
> +        (((index != PCI_ROM_SLOT) ?
> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) &&

The change seems looks good and in line with the commit message. But
this if statement looks like acrobatic circus to me now.


>          (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
>          XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address "
>                      "Register. (addr: 0x%02x, len: %d)\n", addr, len);
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade
>          .offset     = PCI_ROM_ADDRESS,
>          .size       = 4,
>          .init_val   = 0x00000000,
> -        .ro_mask    = 0x000007FE,
> -        .emu_mask   = 0xFFFFF800,
> +        .ro_mask    = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE,
> +        .emu_mask   = (uint32_t)PCI_ROM_ADDRESS_MASK,

There is no need for the explicit cast, is there?


>          .init       = xen_pt_bar_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
>          .u.dw.write = xen_pt_exp_rom_bar_reg_write,
> 
> 
> 

Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Jan Beulich]

>>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote:
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
>>  
>>      /* check unused BAR register */
>>      index = xen_pt_bar_offset_to_index(addr);
>> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
>> +    if ((index >= 0) && (val != 0) &&
>> +        (((index != PCI_ROM_SLOT) ?
>> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) &&
> 
> The change seems looks good and in line with the commit message. But
> this if statement looks like acrobatic circus to me now.

I think the alternative of splitting it up into multiple if()-s would not
be any better readable.

>> --- a/hw/xen/xen_pt_config_init.c
>> +++ b/hw/xen/xen_pt_config_init.c
>> @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade
>>          .offset     = PCI_ROM_ADDRESS,
>>          .size       = 4,
>>          .init_val   = 0x00000000,
>> -        .ro_mask    = 0x000007FE,
>> -        .emu_mask   = 0xFFFFF800,
>> +        .ro_mask    = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE,
>> +        .emu_mask   = (uint32_t)PCI_ROM_ADDRESS_MASK,
> 
> There is no need for the explicit cast, is there?

There is - PCI_ROM_ADDRESS_MASK being wider than 32 bits the
assignment could cause compiler warning otherwise (which I think
I actually ran into.

Jan

Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Stefano Stabellini]

On Fri, 5 Jun 2015, Jan Beulich wrote:
> >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote:
> >> --- a/hw/xen/xen_pt.c
> >> +++ b/hw/xen/xen_pt.c
> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
> >>  
> >>      /* check unused BAR register */
> >>      index = xen_pt_bar_offset_to_index(addr);
> >> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
> >> +    if ((index >= 0) && (val != 0) &&
> >> +        (((index != PCI_ROM_SLOT) ?
> >> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) &&
> > 
> > The change seems looks good and in line with the commit message. But
> > this if statement looks like acrobatic circus to me now.
> 
> I think the alternative of splitting it up into multiple if()-s would not
> be any better readable.

Would you be OK if I rewrote the statement as follows?

    if ((index >= 0) && (val != 0)) {
        uint32_t vu;
        
        if (index == PCI_ROM_SLOT) {
            vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK;
        } else {
            vu = val;
        }

        if ((vu != XEN_PT_BAR_ALLF) &&
            (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
            XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address "
                    "Register. (addr: 0x%02x, len: %d)\n", addr, len);
        }
    }

Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Jan Beulich]

>>> On 05.06.15 at 18:41, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote:
>> >> --- a/hw/xen/xen_pt.c
>> >> +++ b/hw/xen/xen_pt.c
>> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
>> >>  
>> >>      /* check unused BAR register */
>> >>      index = xen_pt_bar_offset_to_index(addr);
>> >> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
>> >> +    if ((index >= 0) && (val != 0) &&
>> >> +        (((index != PCI_ROM_SLOT) ?
>> >> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != 
> XEN_PT_BAR_ALLF) &&
>> > 
>> > The change seems looks good and in line with the commit message. But
>> > this if statement looks like acrobatic circus to me now.
>> 
>> I think the alternative of splitting it up into multiple if()-s would not
>> be any better readable.
> 
> Would you be OK if I rewrote the statement as follows?
> 
>     if ((index >= 0) && (val != 0)) {
>         uint32_t vu;
>         
>         if (index == PCI_ROM_SLOT) {
>             vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK;
>         } else {
>             vu = val;
>         }
> 
>         if ((vu != XEN_PT_BAR_ALLF) &&
>             (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
>             XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address "
>                     "Register. (addr: 0x%02x, len: %d)\n", addr, len);
>         }
>     }

Actually I agree that this indeed looks better. If I were to make the
change, I'd do

    if ((index >= 0) && (val != 0)) {
        uint32_t vu = val;
        
        if (index == PCI_ROM_SLOT) {
            vu |= (uint32_t)~PCI_ROM_ADDRESS_MASK;
        }

        if ((vu != XEN_PT_BAR_ALLF) &&
        ...

though. But if you're going to do the edit without wanting me to
re-submit, I'll certainly leave this to you. Just let me know which
way you prefer it to be handled.

Jan

Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Stefano Stabellini]

On Mon, 8 Jun 2015, Jan Beulich wrote:
> >>> On 05.06.15 at 18:41, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote:
> >> >> --- a/hw/xen/xen_pt.c
> >> >> +++ b/hw/xen/xen_pt.c
> >> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
> >> >>  
> >> >>      /* check unused BAR register */
> >> >>      index = xen_pt_bar_offset_to_index(addr);
> >> >> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
> >> >> +    if ((index >= 0) && (val != 0) &&
> >> >> +        (((index != PCI_ROM_SLOT) ?
> >> >> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != 
> > XEN_PT_BAR_ALLF) &&
> >> > 
> >> > The change seems looks good and in line with the commit message. But
> >> > this if statement looks like acrobatic circus to me now.
> >> 
> >> I think the alternative of splitting it up into multiple if()-s would not
> >> be any better readable.
> > 
> > Would you be OK if I rewrote the statement as follows?
> > 
> >     if ((index >= 0) && (val != 0)) {
> >         uint32_t vu;
> >         
> >         if (index == PCI_ROM_SLOT) {
> >             vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK;
> >         } else {
> >             vu = val;
> >         }
> > 
> >         if ((vu != XEN_PT_BAR_ALLF) &&
> >             (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
> >             XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address "
> >                     "Register. (addr: 0x%02x, len: %d)\n", addr, len);
> >         }
> >     }
> 
> Actually I agree that this indeed looks better. If I were to make the
> change, I'd do
> 
>     if ((index >= 0) && (val != 0)) {
>         uint32_t vu = val;
>         
>         if (index == PCI_ROM_SLOT) {
>             vu |= (uint32_t)~PCI_ROM_ADDRESS_MASK;
>         }
> 
>         if ((vu != XEN_PT_BAR_ALLF) &&
>         ...
> 
> though. But if you're going to do the edit without wanting me to
> re-submit, I'll certainly leave this to you. Just let me know which
> way you prefer it to be handled.

I prefer if you resubmit, thanks!