Re: [Xen-devel] [PATCH v5 1/1] Add mmio_hole_size

["Slutz, Donald Christopher" <dslutz@xxxxxxxxxxx>]

On 09/30/14 09:22, George Dunlap wrote:
> On Tue, Sep 30, 2014 at 2:27 AM, Boris Ostrovsky
> <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> +    if ( mmio_hole_size )
>>> +    {
>>> +        uint64_t max_ram_below_4g = (1ULL << 32) - mmio_hole_size;
>>> +
>>> +        if ( max_ram_below_4g > HVM_BELOW_4G_MMIO_START )
>>> +        {
>>> +            printf("max_ram_below_4g=0x"PRIllx
>>> +                   " too big for mmio_hole_size=0x"PRIllx
>>> +                   " has been ignored.\n",
>>> +                   PRIllx_arg(max_ram_below_4g),
>>> +                   PRIllx_arg(mmio_hole_size));
>>> +        }
>>
>> Do you need to check whether the hole is too large?
>>
>> Here and in the toolstack.
> How large is too large?  I've seen real machines with a 3GiB memory hole...
>
>>> "0";
>>> +        if (info->u.hvm.mmio_hole_size) {
>>> +            uint64_t max_ram_below_4g =
>>> +                (1ULL << 32) - info->u.hvm.mmio_hole_size;
>>> +
>>> +            if (max_ram_below_4g <= HVM_BELOW_4G_MMIO_START) {
>>
>> You seem to be making various checks on hole size in multiple places ---
>> here, in libxl__build_device_model_args_new() and in parse_config_data().
>> (And, to some degree, in xc_hvm_build_with_hole()). Are they all necessary?
> I think we should get rid of xc_hvm_build_with_hole() entirely, but I
> think we want a check everywhere else.  The key question is when the
> failure is going to happen ideally.  You absolutely have to check it
> at the bottom level (hvmloader).  But you don't want to go through all
> the hassle of setting up the domain and starting to boot it only to
> find that some silly parameter is messed up, so it should also be
> checked in libxl (since many people will be going through only libxl).
> But if you're using xl and get an error at the libxl layer, the error
> message is often a bit cryptic.  So it makes sense to have it at the
> parsing level too.
>
>   -George

Ok, I will look into removing xc_hvm_build_with_hole() entirely.

The rest of the statement looks like stuff I should have had in the
commit message.  Are you fine with me adjusting and adding it?

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