Re: [Xen-devel] [PATCH 1/3] xen/tools: Add 64 bits big bar support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.08.12 at 11:24, "Hao, Xudong" <xudong.hao@xxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Thursday, August 16, 2012 7:04 PM
>> To: Hao, Xudong
>> Cc: ian.jackson@xxxxxxxxxxxxx; Zhang, Xiantao; xen-devel@xxxxxxxxxxxxx 
>> Subject: RE: [Xen-devel] [PATCH 1/3] xen/tools: Add 64 bits big bar support
>> 
>> >>> On 16.08.12 at 12:48, "Hao, Xudong" <xudong.hao@xxxxxxxxx> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> >>> On 15.08.12 at 08:54, Xudong Hao <xudong.hao@xxxxxxxxx> wrote:
>> >> > --- a/tools/firmware/hvmloader/config.h Tue Jul 24 17:02:04 2012
>> +0200
>> >> > +++ b/tools/firmware/hvmloader/config.h Thu Jul 26 15:40:01 2012
>> +0800
>> >> > @@ -53,6 +53,10 @@ extern struct bios_config ovmf_config;
>> >> >  /* MMIO hole: Hardcoded defaults, which can be dynamically expanded.
>> */
>> >> >  #define PCI_MEM_START       0xf0000000
>> >> >  #define PCI_MEM_END         0xfc000000
>> >> > +#define PCI_HIGH_MEM_START  0xa000000000ULL
>> >> > +#define PCI_HIGH_MEM_END    0xf000000000ULL
>> >>
>> >> With such hard coded values, this is hardly meant to be anything
>> >> more than an RFC, is it? These values should not exist in the first
>> >> place, and the variables below should be determined from VM
>> >> characteristics (best would presumably be to allocate them top
>> >> down from the end of physical address space, making sure you
>> >> don't run into RAM).
>> 
>> No comment on this part?
>> 
> 
> The MMIO high memory start from 640G, it's already very high, I think we 
> don't need allocate MMIO top down from the high of physical address space. 
> Another thing you remind me that maybe we can skip this high MMIO hole when 
> setup p2m table in build hvm of libxc(setup_guest()), like the handles for 
> MMIO below 4G.

That would be an option, but any fixed address you pick here
will look arbitrary (and will sooner or later raise questions). Plus
by allowing the RAM above 4G to remain contiguous even for
huge guests, we'd retain maximum compatibility with all sorts
of guest OSes. Furthermore, did you check that we in all cases
can use 40-bit (guest) physical addresses (I would think that 36
is the biggest common value). Bottom line - please don't use a
fixed number here.

>> >> > @@ -133,23 +142,35 @@ void pci_setup(void)
>> >> >          /* Map the I/O memory and port resources. */
>> >> >          for ( bar = 0; bar < 7; bar++ )
>> >> >          {
>> >> > +            bar_sz_upper = 0;
>> >> >              bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
>> >> >              if ( bar == 6 )
>> >> >                  bar_reg = PCI_ROM_ADDRESS;
>> >> >
>> >> >              bar_data = pci_readl(devfn, bar_reg);
>> >> > +            is_64bar = !!((bar_data & (PCI_BASE_ADDRESS_SPACE |
>> >> > +                         PCI_BASE_ADDRESS_MEM_TYPE_MASK))
>> ==
>> >> > +                         (PCI_BASE_ADDRESS_SPACE_MEMORY |
>> >> > +                         PCI_BASE_ADDRESS_MEM_TYPE_64));
>> >> >              pci_writel(devfn, bar_reg, ~0);
>> >> >              bar_sz = pci_readl(devfn, bar_reg);
>> >> >              pci_writel(devfn, bar_reg, bar_data);
>> >> > +
>> >> > +            if (is_64bar) {
>> >> > +                bar_data_upper = pci_readl(devfn, bar_reg + 4);
>> >> > +                pci_writel(devfn, bar_reg + 4, ~0);
>> >> > +                bar_sz_upper = pci_readl(devfn, bar_reg + 4);
>> >> > +                pci_writel(devfn, bar_reg + 4, bar_data_upper);
>> >> > +                bar_sz = (bar_sz_upper << 32) | bar_sz;
>> >> > +            }
>> >> > +            bar_sz &= (((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>> >> > +                        PCI_BASE_ADDRESS_SPACE_MEMORY) ?
>> >> > +                       0xfffffffffffffff0 :
>> >>
>> >> This should be a proper constant (or the masking could be
>> >> done earlier, in which case you could continue to use the
>> >> existing PCI_BASE_ADDRESS_MEM_MASK).
>> >>
>> >
>> > So the PCI_BASE_ADDRESS_MEM_MASK can be defined as 'ULL'.
>> 
>> I'd recommend not touching existing variables. As said before,
>> by re-ordering the code you can still use the constant as-is.
>> 
> 
> Mmh, I misunderstood you in the previous mail. 
> So you means we put the mask before "if(is_64bar)", and then we do not do 
> changes for old mask code, right?

Exactly.

Jan

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