Re: [PATCH v2 4/5] x86/vPIT: check values loaded from state save record

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2023 01:37, Andrew Cooper wrote:
> On 16/11/2023 1:47 pm, Jan Beulich wrote:
>> @@ -427,6 +428,47 @@ static int cf_check pit_save(struct vcpu
>>      return rc;
>>  }
>>  
>> +static int cf_check pit_check(const struct domain *d, hvm_domain_context_t 
>> *h)
>> +{
>> +    const struct hvm_hw_pit *hw;
>> +    unsigned int i;
>> +
>> +    if ( !has_vpit(d) )
>> +        return -ENODEV;
>> +
>> +    hw = hvm_point_entry(PIT, h);
>> +    if ( !hw )
>> +        return -ENODATA;
>> +
>> +    /*
>> +     * Check to-be-loaded values are within valid range, for them to 
>> represent
>> +     * actually reachable state.  Uses of some of the values elsewhere 
>> assume
>> +     * this is the case.  Note that the channels' mode fields aren't 
>> checked;
>> +     * older Xen might save them as 0xff.
> 
> "older Xen" goes stale very quickly.  "Xen prior to 4.19", or "Xen prior
> to the time of writing (Nov 2023)" if you're planning to backport this.

Can certainly adjust. And no, I don't think I intend to backport this.

>> @@ -443,6 +485,14 @@ static int cf_check pit_load(struct doma
>>          goto out;
>>      }
>>      
>> +    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
>> +    {
>> +        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
>> +
>> +        if ( (ch->mode &= 7) > 5 )
>> +            ch->mode -= 4;
> 
> How does this work?  If we get in an 0xff, we'll turn it into 0xfb
> rather than 7.

Did you overlook the &= ?

>> @@ -575,7 +625,7 @@ void pit_reset(struct domain *d)
>>      for ( i = 0; i < 3; i++ )
>>      {
>>          s = &pit->hw.channels[i];
>> -        s->mode = 0xff; /* the init mode */
>> +        s->mode = 7; /* the init mode */
> 
> I think it would be helpful to modify the comment to say /* unreachable
> sentinel */ or something.

Can change, sure.

Jan