Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-17 Thread George Dunlap 
On Thu, Jul 16, 2015 at 10:15 PM, Chen, Tiejun tiejun.c...@intel.com wrote:

   base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
 +
 +/* If we're using mem_resource, check for RMRR conflicts */
 +while ( resource == mem_resource 
 +next_rmrr  0 
 +check_overlap(base, bar_sz,
 +  memory_map.map[next_rmrr].addr,
 +  memory_map.map[next_rmrr].size)) {
 +base = memory_map.map[next_rmrr].addr +
 memory_map.map[next_rmrr].size;
 +base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz -
 1);
 +next_rmrr=find_next_rmrr(base);
 +}
 +
   bar_data |= (uint32_t)base;
   bar_data_upper = (uint32_t)(base  32);
   base += bar_sz;


 Actually this chunk of codes are really similar as what we did in my
 previous revisions from RFC ~ v3. It's just trying to skip and then
 allocate, right? As Jan pointed out, there are two key problems:

 #1. All skipping action probably cause a result of no sufficient MMIO to
 allocate all devices as before.

 #2. Another is that alignment issue. When the original base change to
 align to rdm_end, some spaces are wasted. Especially, these spaces could be
 allocated to other smaller bars.

Just to be pedantic: #2 is really just an extension of #1 -- i.e., it
doesn't matter if space is wasted if all the MMIO regions still fit;
the only reason #2 matters is that it makes #1 worse.

In any case, I know it's not perfect -- the point was to get something
that 1) was relatively simple to implement 2) worked out-of-the-box
for many cases, and 3) had a work-around which the user could use in
other cases.

Given that if we run out of MMIO space, all that happens is that some
devices will not really work, I think this solution is really no worse
than the disable devices on conflict solution; and it's better,
because you can actually work around it by increasing the MMIO hole
size.  But I'll leave it to  Jan and others to determine which (if
any) would be suitable to check in at this point.

 -George

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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-17 Thread Jan Beulich 
 On 17.07.15 at 11:26, george.dun...@eu.citrix.com wrote:
 Given that if we run out of MMIO space, all that happens is that some
 devices will not really work, I think this solution is really no worse
 than the disable devices on conflict solution; and it's better,
 because you can actually work around it by increasing the MMIO hole
 size.

I agree.

Jan


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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On Thu, Jul 16, 2015 at 12:52 PM, Chen, Tiejun tiejun.c...@intel.com wrote:
 +for ( i = 0; i  memory_map.nr_map ; i++ )
 +{
 +if ( memory_map.map[i].type != E820_RAM )


 Here we're assuming that any region not marked as RAM is an RMRR.  Is that
 true?

 In any case, it would be just as strange to have a device BAR overlap
 with guest RAM as with an RMRR, wouldn't it?


 OOPS! Actually I should take this,

 if ( memory_map.map[i].type == E820_RESERVED )

 This is same as when I check [RESERVED_MEMORY_DYNAMIC_START,
 RESERVED_MEMORY_DYNAMIC_END).



 +{
 +uint64_t reserved_start, reserved_size;
 +reserved_start = memory_map.map[i].addr;
 +reserved_size = memory_map.map[i].size;
 +if ( check_overlap(bar_data , bar_sz,
 +   reserved_start, reserved_size) )
 +{
 +is_conflict = true;
 +/* Now disable the memory or I/O mapping. */
 +printf(pci dev %02x:%x bar %02x : 0x%08x :
 conflicts 
 +   reserved resource so disable this
 device.!\n,
 +   devfn3, devfn7, bar_reg, bar_data);
 +cmd = pci_readw(devfn, PCI_COMMAND);
 +pci_writew(devfn, PCI_COMMAND, ~cmd);
 +break;
 +}
 +}
 +
 +/* Jump next device. */
 +if ( is_conflict )
 +{
 +is_conflict = false;
 +break;
 +}


 This conditional is still inside the memory_map loop; you want it one
 loop futher out, in the bar loop, don't you?


 Here what I intended to do is if one of all bars specific to one given
 device already conflicts with RDM, its not necessary to continue check other
 remaining bars of this device and other RDM regions, we just disable this
 device simply then check next device.

I know what you're trying to do; what I'm saying is I don't think it
does what you want it to do.

You have loops nested 3 deep:
1. for each dev
  2.  for each bar
3. for each memory range

This conditional is in loop 3; you want it to be in loop 2.

(In fact, when you set is_conflict, you then break out of loop 3 back
into loop 2; so this code will never actually be run.)

  Also, if you declare is_conflict inside the devfn loop, rather than in
 the main function, then you don't need this is_conflict=false here.

 It might also be more sensible to use a goto instead; but this is one


 This can work for me so it may be as follows:

 for ( devfn = 0; devfn  256; devfn++ )
 {
  check_next_device:
 vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
 device_id = pci_readw(devfn, PCI_DEVICE_ID);
 if ( (vendor_id == 0x)  (device_id == 0x) )
 continue;
 ...
 if ( check_overlap(bar_data , bar_sz,
reserved_start, reserved_size) )
 {
 ...
 /* Jump next device. */
 devfn++;
 goto check_next_device;
 }

I'm not a fan of hard-coding the loop continuing condition like this;
if I were going to do a goto, I'd want to go to the end of the loop.

Anyway, the code is OK as it is; I'd rather spend time working on
something that's more of a blocker.

 -George

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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Jan Beulich 
 On 16.07.15 at 15:21, tiejun.c...@intel.com wrote:
 I guess something like this,
 
   ...
  pci_writew(devfn, PCI_COMMAND, ~cmd);
  /* Jump next device. */
  goto check_next_device;
  }
  }
  }
  }
   check_next_device:
  }
 }

Except that this isn't valid C (no statement following the label). I can
accept goto-s for some error handling cases where the alternatives
might be considered even more ugly than using goto. But the way
this or your original proposal look, I'd rather not have goto-s used
like this.

Jan


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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On Thu, Jul 16, 2015 at 7:52 AM, Tiejun Chen tiejun.c...@intel.com wrote:
 When allocating mmio address for PCI bars, mmio may overlap with
 reserved regions. Currently we just want to disable these associate
 devices simply to avoid conflicts but we will reshape current mmio
 allocation mechanism to fix this completely.

On the whole I still think it would be good to try to relocate BARs if
possible; I would be OK with this if there isn't a better option.

A couple of comments on the patch, however:


 CC: Keir Fraser k...@xen.org
 CC: Jan Beulich jbeul...@suse.com
 CC: Andrew Cooper andrew.coop...@citrix.com
 CC: Ian Jackson ian.jack...@eu.citrix.com
 CC: Stefano Stabellini stefano.stabell...@eu.citrix.com
 CC: Ian Campbell ian.campb...@citrix.com
 CC: Wei Liu wei.l...@citrix.com
 Signed-off-by: Tiejun Chen tiejun.c...@intel.com
 ---
 v8:

 * Based on current discussion its hard to reshape the original mmio
   allocation mechanism but we haven't a good and simple way to in short term.
   So instead, we don't bring more complicated to intervene that process but
   still check any conflicts to disable all associated devices.

 v6 ~ v7:

 * Nothing is changed.

 v5:

 * Rename that field, is_64bar, inside struct bars with flag, and
   then extend to also indicate if this bar is already allocated.

 v4:

 * We have to re-design this as follows:

   #1. Goal

   MMIO region should exclude all reserved device memory

   #2. Requirements

   #2.1 Still need to make sure MMIO region is fit all pci devices as before

   #2.2 Accommodate the not aligned reserved memory regions

   If I'm missing something let me know.

   #3. How to

   #3.1 Address #2.1

   We need to either of populating more RAM, or of expanding more highmem. But
   we should know just 64bit-bar can work with highmem, and as you mentioned we
   also should avoid expanding highmem as possible. So my implementation is to
   allocate 32bit-bar and 64bit-bar orderly.

   1. The first allocation round just to 32bit-bar

   If we can finish allocating all 32bit-bar, we just go to allocate 64bit-bar
   with all remaining resources including low pci memory.

   If not, we need to calculate how much RAM should be populated to allocate 
 the
   remaining 32bit-bars, then populate sufficient RAM as exp_mem_resource to go
   to the second allocation round 2.

   2. The second allocation round to the remaining 32bit-bar

   We should can finish allocating all 32bit-bar in theory, then go to the 
 third
   allocation round 3.

   3. The third allocation round to 64bit-bar

   We'll try to first allocate from the remaining low memory resource. If that
   isn't enough, we try to expand highmem to allocate for 64bit-bar. This 
 process
   should be same as the original.

   #3.2 Address #2.2

   I'm trying to accommodate the not aligned reserved memory regions:

   We should skip all reserved device memory, but we also need to check if 
 other
   smaller bars can be allocated if a mmio hole exists between resource-base 
 and
   reserved device memory. If a hole exists between base and reserved device
   memory, lets go out simply to try allocate for next bar since all bars are 
 in
   descending order of size. If not, we need to move resource-base to 
 reserved_end
   just to reallocate this bar.

  tools/firmware/hvmloader/pci.c | 87 
 ++
  1 file changed, 87 insertions(+)

 diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
 index 5ff87a7..9e017d5 100644
 --- a/tools/firmware/hvmloader/pci.c
 +++ b/tools/firmware/hvmloader/pci.c
 @@ -38,6 +38,90 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
  enum virtual_vga virtual_vga = VGA_none;
  unsigned long igd_opregion_pgbase = 0;

 +/*
 + * We should check if all valid bars conflict with RDM.
 + *
 + * Here we just need to check mmio bars in the case of non-highmem
 + * since the hypervisor can make sure RDM doesn't involve highmem.
 + */
 +static void disable_conflicting_devices(void)
 +{
 +uint8_t is_64bar;
 +uint32_t devfn, bar_reg, cmd, bar_data;
 +uint16_t vendor_id, device_id;
 +unsigned int bar, i;
 +uint64_t bar_sz;
 +bool is_conflict = false;
 +
 +for ( devfn = 0; devfn  256; devfn++ )
 +{
 +vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
 +device_id = pci_readw(devfn, PCI_DEVICE_ID);
 +if ( (vendor_id == 0x)  (device_id == 0x) )
 +continue;
 +
 +/* Check all bars */
 +for ( bar = 0; bar  7; bar++ )
 +{
 +bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
 +if ( bar == 6 )
 +bar_reg = PCI_ROM_ADDRESS;
 +
 +bar_data = pci_readl(devfn, bar_reg);
 +bar_data = PCI_BASE_ADDRESS_MEM_MASK;
 +if ( !bar_data )
 +continue;
 +
 +if ( bar_reg != PCI_ROM_ADDRESS )
 +is_64bar = !!((bar_data  (PCI_BASE_ADDRESS_SPACE |
 +

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

  Except that this isn't valid C (no statement following the label). I can

accept goto-s for some error handling cases where the alternatives
might be considered even more ugly than using goto. But the way
this or your original proposal look, I'd rather not have goto-s used
like this.



What about this?

+bool is_conflict = false;

 for ( devfn = 0; devfn  256; devfn++ )
 {
@@ -60,7 +61,7 @@ static void disable_conflicting_devices(void)
 continue;

 /* Check all bars */
-for ( bar = 0; bar  7; bar++ )
+for ( bar = 0; bar  7  !is_conflict; bar++ )
 {
 bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
 if ( bar == 6 )
@@ -89,7 +90,7 @@ static void disable_conflicting_devices(void)
 bar_sz = pci_readl(devfn, bar_reg);
 bar_sz = PCI_BASE_ADDRESS_MEM_MASK;

-for ( i = 0; i  memory_map.nr_map ; i++ )
+for ( i = 0; i  memory_map.nr_map  !is_conflict; i++ )
 {
 if ( memory_map.map[i].type == E820_RESERVED )
 {
@@ -105,13 +106,13 @@ static void disable_conflicting_devices(void)
devfn3, devfn7, bar_reg, bar_data);
 cmd = pci_readw(devfn, PCI_COMMAND);
 pci_writew(devfn, PCI_COMMAND, ~cmd);
-/* Jump next device. */
-goto check_next_device;
+/* So need to jump next device. */
+is_conflict = true;
 }
 }
 }
 }
- check_next_device:
+is_conflict = false;
 }
 }

Thanks
Tiejun

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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

+for ( i = 0; i  memory_map.nr_map ; i++ )
+{
+if ( memory_map.map[i].type != E820_RAM )


Here we're assuming that any region not marked as RAM is an RMRR.  Is that true?

In any case, it would be just as strange to have a device BAR overlap
with guest RAM as with an RMRR, wouldn't it?


OOPS! Actually I should take this,

if ( memory_map.map[i].type == E820_RESERVED )

This is same as when I check [RESERVED_MEMORY_DYNAMIC_START, 
RESERVED_MEMORY_DYNAMIC_END).





+{
+uint64_t reserved_start, reserved_size;
+reserved_start = memory_map.map[i].addr;
+reserved_size = memory_map.map[i].size;
+if ( check_overlap(bar_data , bar_sz,
+   reserved_start, reserved_size) )
+{
+is_conflict = true;
+/* Now disable the memory or I/O mapping. */
+printf(pci dev %02x:%x bar %02x : 0x%08x : conflicts 
+   reserved resource so disable this device.!\n,
+   devfn3, devfn7, bar_reg, bar_data);
+cmd = pci_readw(devfn, PCI_COMMAND);
+pci_writew(devfn, PCI_COMMAND, ~cmd);
+break;
+}
+}
+
+/* Jump next device. */
+if ( is_conflict )
+{
+is_conflict = false;
+break;
+}


This conditional is still inside the memory_map loop; you want it one
loop futher out, in the bar loop, don't you?


Here what I intended to do is if one of all bars specific to one given 
device already conflicts with RDM, its not necessary to continue check 
other remaining bars of this device and other RDM regions, we just 
disable this device simply then check next device.




Also, if you declare is_conflict inside the devfn loop, rather than in
the main function, then you don't need this is_conflict=false here.

It might also be more sensible to use a goto instead; but this is one


This can work for me so it may be as follows:

for ( devfn = 0; devfn  256; devfn++ )
{
 check_next_device:
vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
device_id = pci_readw(devfn, PCI_DEVICE_ID);
if ( (vendor_id == 0x)  (device_id == 0x) )
continue;
...
if ( check_overlap(bar_data , bar_sz,
   reserved_start, reserved_size) )
{
...
/* Jump next device. */
devfn++;
goto check_next_device;
}



where Jan will have a better idea what standard practice will be.



I can follow that again if Jan has any good implementation.

Thanks
Tiejun

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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Jan Beulich 
   On 16.07.15 at 15:48, tiejun.c...@intel.com wrote:
  Except that this isn't valid C (no statement following the label). I can
 accept goto-s for some error handling cases where the alternatives
 might be considered even more ugly than using goto. But the way
 this or your original proposal look, I'd rather not have goto-s used
 like this.

 
 What about this?

Looks reasonable (but don't forget that I continue to be unconvinced
that the patch as a whole makes sense).

Jan

 +bool is_conflict = false;
 
   for ( devfn = 0; devfn  256; devfn++ )
   {
 @@ -60,7 +61,7 @@ static void disable_conflicting_devices(void)
   continue;
 
   /* Check all bars */
 -for ( bar = 0; bar  7; bar++ )
 +for ( bar = 0; bar  7  !is_conflict; bar++ )
   {
   bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
   if ( bar == 6 )
 @@ -89,7 +90,7 @@ static void disable_conflicting_devices(void)
   bar_sz = pci_readl(devfn, bar_reg);
   bar_sz = PCI_BASE_ADDRESS_MEM_MASK;
 
 -for ( i = 0; i  memory_map.nr_map ; i++ )
 +for ( i = 0; i  memory_map.nr_map  !is_conflict; i++ )
   {
   if ( memory_map.map[i].type == E820_RESERVED )
   {
 @@ -105,13 +106,13 @@ static void disable_conflicting_devices(void)
  devfn3, devfn7, bar_reg, bar_data);
   cmd = pci_readw(devfn, PCI_COMMAND);
   pci_writew(devfn, PCI_COMMAND, ~cmd);
 -/* Jump next device. */
 -goto check_next_device;
 +/* So need to jump next device. */
 +is_conflict = true;
   }
   }
   }
   }
 - check_next_device:
 +is_conflict = false;
   }
   }
 
 Thanks
 Tiejun




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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

What about this?


Looks reasonable (but don't forget that I continue to be unconvinced
that the patch as a whole makes sense).


Yes, I always keep this in my mind as I mentioned in patch #00. Any risk 
you're still concerning? Is it that case if guest OS force enabling 
these devices again? IMO, at this point there are two cases:


#1. Without passing through a RMRR device

Those emulated devices don't create 1:1 mapping so its safe, right?

#2. With passing through a RMRR device

This just probably cause these associated devices not to work well, but 
still don't bring any impact to other Domains, right? I mean this isn't 
going to worsen the preexisting situation.


If I'm wrong please correct me.

Thanks
Tiejun



Jan


+bool is_conflict = false;

   for ( devfn = 0; devfn  256; devfn++ )
   {
@@ -60,7 +61,7 @@ static void disable_conflicting_devices(void)
   continue;

   /* Check all bars */
-for ( bar = 0; bar  7; bar++ )
+for ( bar = 0; bar  7  !is_conflict; bar++ )
   {
   bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
   if ( bar == 6 )
@@ -89,7 +90,7 @@ static void disable_conflicting_devices(void)
   bar_sz = pci_readl(devfn, bar_reg);
   bar_sz = PCI_BASE_ADDRESS_MEM_MASK;

-for ( i = 0; i  memory_map.nr_map ; i++ )
+for ( i = 0; i  memory_map.nr_map  !is_conflict; i++ )
   {
   if ( memory_map.map[i].type == E820_RESERVED )
   {
@@ -105,13 +106,13 @@ static void disable_conflicting_devices(void)
  devfn3, devfn7, bar_reg, bar_data);
   cmd = pci_readw(devfn, PCI_COMMAND);
   pci_writew(devfn, PCI_COMMAND, ~cmd);
-/* Jump next device. */
-goto check_next_device;
+/* So need to jump next device. */
+is_conflict = true;
   }
   }
   }
   }
- check_next_device:
+is_conflict = false;
   }
   }

Thanks
Tiejun







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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On 07/16/2015 04:20 PM, Chen, Tiejun wrote:
 What about this?

 Looks reasonable (but don't forget that I continue to be unconvinced
 that the patch as a whole makes sense).
 
 Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
 you're still concerning? Is it that case if guest OS force enabling
 these devices again? IMO, at this point there are two cases:
 
 #1. Without passing through a RMRR device
 
 Those emulated devices don't create 1:1 mapping so its safe, right?
 
 #2. With passing through a RMRR device
 
 This just probably cause these associated devices not to work well, but
 still don't bring any impact to other Domains, right? I mean this isn't
 going to worsen the preexisting situation.
 
 If I'm wrong please correct me.

But I think the issue is, without doing *something* about MMIO
collisions, the feature as a whole is sort of pointless.  You can
carefully specify rdm=strategy=host,reserved=strict, but you might
still get devices whose MMIO regions conflict with RMMRs, and there's
nothing you can really do about it.

And although I personally think it might be possible / reasonable to
check in a newly-written, partial MMIO collision avoidance patch, not
everyone might agree.  Even if I were to rewrite and post a patch
myself, they may argue that doing such a complicated re-design after the
feature freeze shouldn't be allowed.

 -George

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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

Here what I intended to do is if one of all bars specific to one given
device already conflicts with RDM, its not necessary to continue check other
remaining bars of this device and other RDM regions, we just disable this
device simply then check next device.


I know what you're trying to do; what I'm saying is I don't think it
does what you want it to do.

You have loops nested 3 deep:
1. for each dev
   2.  for each bar
 3. for each memory range

This conditional is in loop 3; you want it to be in loop 2.

(In fact, when you set is_conflict, you then break out of loop 3 back
into loop 2; so this code will never actually be run.)


Sorry I should make this clear last time.

I mean I already knew what you were saying is right at this point so I 
tried to use goto to fix this bug.




   Also, if you declare is_conflict inside the devfn loop, rather than in

the main function, then you don't need this is_conflict=false here.

It might also be more sensible to use a goto instead; but this is one





[snip]


I'm not a fan of hard-coding the loop continuing condition like this;
if I were going to do a goto, I'd want to go to the end of the loop.



I guess something like this,

...
pci_writew(devfn, PCI_COMMAND, ~cmd);
/* Jump next device. */
goto check_next_device;
}
}
}
}
 check_next_device:
}
}


Anyway, the code is OK as it is; I'd rather spend time working on
something that's more of a blocker.



Thanks
Tiejun



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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On Thu, Jul 16, 2015 at 5:18 PM, George Dunlap
george.dun...@eu.citrix.com wrote:
 On Thu, Jul 16, 2015 at 4:39 PM, George Dunlap george.dun...@citrix.com 
 wrote:
 On 07/16/2015 04:20 PM, Chen, Tiejun wrote:
 What about this?

 Looks reasonable (but don't forget that I continue to be unconvinced
 that the patch as a whole makes sense).

 Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
 you're still concerning? Is it that case if guest OS force enabling
 these devices again? IMO, at this point there are two cases:

 #1. Without passing through a RMRR device

 Those emulated devices don't create 1:1 mapping so its safe, right?

 #2. With passing through a RMRR device

 This just probably cause these associated devices not to work well, but
 still don't bring any impact to other Domains, right? I mean this isn't
 going to worsen the preexisting situation.

 If I'm wrong please correct me.

 But I think the issue is, without doing *something* about MMIO
 collisions, the feature as a whole is sort of pointless.  You can
 carefully specify rdm=strategy=host,reserved=strict, but you might
 still get devices whose MMIO regions conflict with RMMRs, and there's
 nothing you can really do about it.

 And although I personally think it might be possible / reasonable to
 check in a newly-written, partial MMIO collision avoidance patch, not
 everyone might agree.  Even if I were to rewrite and post a patch
 myself, they may argue that doing such a complicated re-design after the
 feature freeze shouldn't be allowed.

 What about something like this?

  -George

 ---
  [PATCH] hvmloader/pci: Try to avoid placing BARs in RMRRs

 Try to avoid placing PCI BARs over RMRRs:

 - If mmio_hole_size is not specified, and the existing MMIO range has
   RMRRs in it, and there is space to expand the hole in lowmem without
   moving more memory, then make the MMIO hole as large as possible.

 - When placing RMRRs, find the next RMRR higher than the current base
   in the lowmem mmio hole.  If it overlaps, skip ahead of it and find
   the next one.

 This certainly won't work in all cases, but it should work in a
 significant number of cases.  Additionally, users should be able to
 work around problems by setting mmio_hole_size larger in the guest
 config.

 Signed-off-by: George Dunlap george.dun...@eu.citrix.com
 ---
 THIS WILL NOT COMPILE, as it needs check_overlap_all() to be implemented.

 It's just a proof-of-concept for discussion.
 ---
  tools/firmware/hvmloader/pci.c | 42 
 ++
  1 file changed, 42 insertions(+)

 diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
 index 5ff87a7..dcb8cd0 100644
 --- a/tools/firmware/hvmloader/pci.c
 +++ b/tools/firmware/hvmloader/pci.c
 @@ -38,6 +38,25 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
  enum virtual_vga virtual_vga = VGA_none;
  unsigned long igd_opregion_pgbase = 0;

 +/* Find the lowest RMRR higher than base */
 +int find_next_rmrr(uint32_t base)
 +{
 +int next_rmrr=-1;
 +uing64_t min_base = (1ull  32);
 +
 +for ( i = 0; i  memory_map.nr_map ; i++ )
 +{
 +if ( memory_map.map[i].type == E820_RESERVED
 +  memory_map.map[i].addr  base
 +  memory_map.map[i].addr  min_base)
 +{
 +next_rmrr = i;
 +min_base = memory_map.map[i].addr;
 +}
 +}
 +return next_rmrr;
 +}
 +
  void pci_setup(void)
  {
  uint8_t is_64bar, using_64bar, bar64_relocate = 0;
 @@ -299,6 +318,15 @@ void pci_setup(void)
  || (((pci_mem_start  1)  PAGE_SHIFT)
  = hvm_info-low_mem_pgend)) )
  pci_mem_start = 1;
 +
 +/*
 + * Try to accomodate RMRRs in our MMIO region on a best-effort basis.
 + * If we have RMRRs in the range, then make pci_mem_start just after
 + * hvm_info-low_mem_pgend.
 + */
 +if ( pci_mem_start  (hvm_info-low_mem_pgend  PAGE_SHIFT) 
 + check_overlap_all(pci_mem_start, pci_mem_end-pci_mem_start) )
 +pci_mem_start = (hvm_info-low_mem_pgend + 1 )  PAGE_SHIFT);
  }

  if ( mmio_total  (pci_mem_end - pci_mem_start) )
 @@ -352,6 +380,8 @@ void pci_setup(void)
  io_resource.base = 0xc000;
  io_resource.max = 0x1;

 +next_rmrr = find_next_rmrr(pci_mem_start);
 +
  /* Assign iomem and ioport resources in descending order of size. */
  for ( i = 0; i  nr_bars; i++ )
  {
 @@ -407,6 +437,18 @@ void pci_setup(void)
  }

  base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
 +
 +/* If we're using mem_resource, check for RMRR conflicts */
 +while ( resource == mem_resource 
 +next_rmrr  0 
 +check_overlap(base, bar_sz,
 +  memory_map.map[next_rmrr].addr,
 +  memory_map.map[next_rmrr].size)) {
 +base =

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On 07/16/2015 05:08 PM, Chen, Tiejun wrote:
 On 2015/7/16 23:39, George Dunlap wrote:
 On 07/16/2015 04:20 PM, Chen, Tiejun wrote:
 What about this?

 Looks reasonable (but don't forget that I continue to be unconvinced
 that the patch as a whole makes sense).

 Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
 you're still concerning? Is it that case if guest OS force enabling
 these devices again? IMO, at this point there are two cases:

 #1. Without passing through a RMRR device

 Those emulated devices don't create 1:1 mapping so its safe, right?

 #2. With passing through a RMRR device

 This just probably cause these associated devices not to work well, but
 still don't bring any impact to other Domains, right? I mean this isn't
 going to worsen the preexisting situation.

 If I'm wrong please correct me.

 But I think the issue is, without doing *something* about MMIO
 collisions, the feature as a whole is sort of pointless.  You can
 carefully specify rdm=strategy=host,reserved=strict, but you might
 
 I got what your mean. But there's no a good approach to bridge between
 xl and hvmloader to follow this policy. Right now, maybe just one thing
 could be tried like this,
 
 Under hvmloader circumstance,
 
 strict - Still set RDM as E820_RESERVED
 relaxed - Set RDM as a new internal E820 flag like E820_HAZARDOUS
 
 Then in the case of MMIO collisions
 
 E820_RESERVED - BUG() - Stop VM
 E820_HAZARDOUS - our warning messages + disable devices
 
 I think this can make sure we always take consistent policy in each
 involved cycle.

A better way to communicate between xl and hvmloader is to use xenstore,
as we do for allow_memory_reallocate.  But I have very little hope we
can hash out a suitable design for that by tomorrow.

 -George


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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

On 2015/7/16 23:39, George Dunlap wrote:

On 07/16/2015 04:20 PM, Chen, Tiejun wrote:

What about this?


Looks reasonable (but don't forget that I continue to be unconvinced
that the patch as a whole makes sense).


Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
you're still concerning? Is it that case if guest OS force enabling
these devices again? IMO, at this point there are two cases:

#1. Without passing through a RMRR device

Those emulated devices don't create 1:1 mapping so its safe, right?

#2. With passing through a RMRR device

This just probably cause these associated devices not to work well, but
still don't bring any impact to other Domains, right? I mean this isn't
going to worsen the preexisting situation.

If I'm wrong please correct me.


But I think the issue is, without doing *something* about MMIO
collisions, the feature as a whole is sort of pointless.  You can
carefully specify rdm=strategy=host,reserved=strict, but you might


I got what your mean. But there's no a good approach to bridge between 
xl and hvmloader to follow this policy. Right now, maybe just one thing 
could be tried like this,


Under hvmloader circumstance,

strict - Still set RDM as E820_RESERVED
relaxed - Set RDM as a new internal E820 flag like E820_HAZARDOUS

Then in the case of MMIO collisions

E820_RESERVED - BUG() - Stop VM
E820_HAZARDOUS - our warning messages + disable devices

I think this can make sure we always take consistent policy in each 
involved cycle.


Thanks
Tiejun


still get devices whose MMIO regions conflict with RMMRs, and there's
nothing you can really do about it.

And although I personally think it might be possible / reasonable to
check in a newly-written, partial MMIO collision avoidance patch, not
everyone might agree.  Even if I were to rewrite and post a patch
myself, they may argue that doing such a complicated re-design after the
feature freeze shouldn't be allowed.

  -George



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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread George Dunlap 
On Thu, Jul 16, 2015 at 4:39 PM, George Dunlap george.dun...@citrix.com wrote:
 On 07/16/2015 04:20 PM, Chen, Tiejun wrote:
 What about this?

 Looks reasonable (but don't forget that I continue to be unconvinced
 that the patch as a whole makes sense).

 Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
 you're still concerning? Is it that case if guest OS force enabling
 these devices again? IMO, at this point there are two cases:

 #1. Without passing through a RMRR device

 Those emulated devices don't create 1:1 mapping so its safe, right?

 #2. With passing through a RMRR device

 This just probably cause these associated devices not to work well, but
 still don't bring any impact to other Domains, right? I mean this isn't
 going to worsen the preexisting situation.

 If I'm wrong please correct me.

 But I think the issue is, without doing *something* about MMIO
 collisions, the feature as a whole is sort of pointless.  You can
 carefully specify rdm=strategy=host,reserved=strict, but you might
 still get devices whose MMIO regions conflict with RMMRs, and there's
 nothing you can really do about it.

 And although I personally think it might be possible / reasonable to
 check in a newly-written, partial MMIO collision avoidance patch, not
 everyone might agree.  Even if I were to rewrite and post a patch
 myself, they may argue that doing such a complicated re-design after the
 feature freeze shouldn't be allowed.

What about something like this?

 -George

---
 [PATCH] hvmloader/pci: Try to avoid placing BARs in RMRRs

Try to avoid placing PCI BARs over RMRRs:

- If mmio_hole_size is not specified, and the existing MMIO range has
  RMRRs in it, and there is space to expand the hole in lowmem without
  moving more memory, then make the MMIO hole as large as possible.

- When placing RMRRs, find the next RMRR higher than the current base
  in the lowmem mmio hole.  If it overlaps, skip ahead of it and find
  the next one.

This certainly won't work in all cases, but it should work in a
significant number of cases.  Additionally, users should be able to
work around problems by setting mmio_hole_size larger in the guest
config.

Signed-off-by: George Dunlap george.dun...@eu.citrix.com
---
THIS WILL NOT COMPILE, as it needs check_overlap_all() to be implemented.

It's just a proof-of-concept for discussion.
---
 tools/firmware/hvmloader/pci.c | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 5ff87a7..dcb8cd0 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,6 +38,25 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 enum virtual_vga virtual_vga = VGA_none;
 unsigned long igd_opregion_pgbase = 0;

+/* Find the lowest RMRR higher than base */
+int find_next_rmrr(uint32_t base)
+{
+int next_rmrr=-1;
+uing64_t min_base = (1ull  32);
+
+for ( i = 0; i  memory_map.nr_map ; i++ )
+{
+if ( memory_map.map[i].type == E820_RESERVED
+  memory_map.map[i].addr  base
+  memory_map.map[i].addr  min_base)
+{
+next_rmrr = i;
+min_base = memory_map.map[i].addr;
+}
+}
+return next_rmrr;
+}
+
 void pci_setup(void)
 {
 uint8_t is_64bar, using_64bar, bar64_relocate = 0;
@@ -299,6 +318,15 @@ void pci_setup(void)
 || (((pci_mem_start  1)  PAGE_SHIFT)
 = hvm_info-low_mem_pgend)) )
 pci_mem_start = 1;
+
+/*
+ * Try to accomodate RMRRs in our MMIO region on a best-effort basis.
+ * If we have RMRRs in the range, then make pci_mem_start just after
+ * hvm_info-low_mem_pgend.
+ */
+if ( pci_mem_start  (hvm_info-low_mem_pgend  PAGE_SHIFT) 
+ check_overlap_all(pci_mem_start, pci_mem_end-pci_mem_start) )
+pci_mem_start = (hvm_info-low_mem_pgend + 1 )  PAGE_SHIFT);
 }

 if ( mmio_total  (pci_mem_end - pci_mem_start) )
@@ -352,6 +380,8 @@ void pci_setup(void)
 io_resource.base = 0xc000;
 io_resource.max = 0x1;

+next_rmrr = find_next_rmrr(pci_mem_start);
+
 /* Assign iomem and ioport resources in descending order of size. */
 for ( i = 0; i  nr_bars; i++ )
 {
@@ -407,6 +437,18 @@ void pci_setup(void)
 }

 base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
+
+/* If we're using mem_resource, check for RMRR conflicts */
+while ( resource == mem_resource 
+next_rmrr  0 
+check_overlap(base, bar_sz,
+  memory_map.map[next_rmrr].addr,
+  memory_map.map[next_rmrr].size)) {
+base = memory_map.map[next_rmrr].addr +
memory_map.map[next_rmrr].size;
+base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
+next_rmrr=find_next_rmrr(base);
+

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 

Jan and George,

That implementation to this problem in v7 is really not accepted? Yes, 
that isn't a best solution to make our original mechanism very well, but 
in high level I just think that should be a correct solution fixing this 
problem. According to recent discussion seems we have not a efficient 
way which can be put into 4.6. So instead, could your guys help me make 
that better gradually to reach our current requirement as possible?


Thanks
Tiejun

On 2015/7/17 0:40, George Dunlap wrote:

On 07/16/2015 05:08 PM, Chen, Tiejun wrote:

On 2015/7/16 23:39, George Dunlap wrote:

On 07/16/2015 04:20 PM, Chen, Tiejun wrote:

What about this?


Looks reasonable (but don't forget that I continue to be unconvinced
that the patch as a whole makes sense).


Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
you're still concerning? Is it that case if guest OS force enabling
these devices again? IMO, at this point there are two cases:

#1. Without passing through a RMRR device

Those emulated devices don't create 1:1 mapping so its safe, right?

#2. With passing through a RMRR device

This just probably cause these associated devices not to work well, but
still don't bring any impact to other Domains, right? I mean this isn't
going to worsen the preexisting situation.

If I'm wrong please correct me.


But I think the issue is, without doing *something* about MMIO
collisions, the feature as a whole is sort of pointless.  You can
carefully specify rdm=strategy=host,reserved=strict, but you might


I got what your mean. But there's no a good approach to bridge between
xl and hvmloader to follow this policy. Right now, maybe just one thing
could be tried like this,

Under hvmloader circumstance,

strict - Still set RDM as E820_RESERVED
relaxed - Set RDM as a new internal E820 flag like E820_HAZARDOUS

Then in the case of MMIO collisions

E820_RESERVED - BUG() - Stop VM
E820_HAZARDOUS - our warning messages + disable devices

I think this can make sure we always take consistent policy in each
involved cycle.


A better way to communicate between xl and hvmloader is to use xenstore,
as we do for allow_memory_reallocate.  But I have very little hope we
can hash out a suitable design for that by tomorrow.

  -George




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

Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

2015-07-16 Thread Chen, Tiejun 


  base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
+
+/* If we're using mem_resource, check for RMRR conflicts */
+while ( resource == mem_resource 
+next_rmrr  0 
+check_overlap(base, bar_sz,
+  memory_map.map[next_rmrr].addr,
+  memory_map.map[next_rmrr].size)) {
+base = memory_map.map[next_rmrr].addr +
memory_map.map[next_rmrr].size;
+base = (resource-base  + bar_sz - 1)  ~(uint64_t)(bar_sz - 1);
+next_rmrr=find_next_rmrr(base);
+}
+
  bar_data |= (uint32_t)base;
  bar_data_upper = (uint32_t)(base  32);
  base += bar_sz;



Actually this chunk of codes are really similar as what we did in my 
previous revisions from RFC ~ v3. It's just trying to skip and then 
allocate, right? As Jan pointed out, there are two key problems:


#1. All skipping action probably cause a result of no sufficient MMIO to 
allocate all devices as before.


#2. Another is that alignment issue. When the original base change to 
align to rdm_end, some spaces are wasted. Especially, these spaces could 
be allocated to other smaller bars.


This is one key reason why I had new revision started from v4 to address 
these two points :)


Thanks
Tiejun

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