diff mbox

xen/pass-through: ROM BAR handling adjustments

Message ID 5556059F020000780007A8D0@mail.emea.novell.com
State New
Headers show

Commit Message

Jan Beulich May 15, 2015, 12:41 p.m. UTC 
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>

Comments

Jan Beulich June 1, 2015, 6:55 a.m. UTC | #1 
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
Stefano Stabellini June 5, 2015, 11:32 a.m. UTC | #2 
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,
> 
> 
>
Jan Beulich June 5, 2015, 11:40 a.m. UTC | #3 
>>> 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
Stefano Stabellini June 5, 2015, 4:41 p.m. UTC | #4 
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);
        }
    }
Jan Beulich June 8, 2015, 7:11 a.m. UTC | #5 
>>> 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
Stefano Stabellini June 8, 2015, 11:35 a.m. UTC | #6 
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!
diff mbox

Patch

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